Skip to content

Update to the vault script#109

Closed
Streator wants to merge 17 commits intomainfrom
script-updates
Closed

Update to the vault script#109
Streator wants to merge 17 commits intomainfrom
script-updates

Conversation

@Streator
Copy link
Copy Markdown
Contributor

@Streator Streator commented Sep 9, 2025 •
edited by qodo-code-review bot
Loading

PR Type

Enhancement

Description

  • Add comprehensive vault deployment scripts for Symbiotic protocol

  • Support both regular and tokenized vault deployments

  • Include role-based access control validation

  • Provide configurable parameters for delegators and slashers

Diagram Walkthrough

flowchart LR
  A["DeployVault.s.sol"] --> B["DeployVaultBase.sol"]
  C["DeployVaultTokenized.s.sol"] --> D["DeployVaultTokenizedBase.sol"]
  D --> B
  B --> E["Logs.sol"]
  B --> F["Vault Creation"]
  F --> G["Role Validation"]
Loading

File Walkthrough
Relevant files
Enhancement
DeployVault.s.sol
Regular vault deployment script implementation                     

script/DeployVault.s.sol

  • Defines configuration constants for vault deployment
  • Implements run() function calling base deployment logic
  • Supports multiple delegator and slasher types
  • Includes optional parameters for deposit limits and whitelisting
 +91/-0   
DeployVaultTokenized.s.sol
Tokenized vault deployment script implementation                 

script/DeployVaultTokenized.s.sol

  • Extends regular vault deployment with ERC20 tokenization
  • Adds NAME and SYMBOL constants for token metadata
  • Inherits all configuration options from base vault
  • Calls tokenized base deployment logic
 +99/-0   
DeployVaultBase.sol
Base vault deployment logic with validation                           

script/base/DeployVaultBase.sol

  • Core deployment logic with parameter encoding
  • Comprehensive role validation after deployment
  • Support for 4 delegator types and 2 slasher types
  • Handles whitelist depositor setup and role transfers
 +356/-0 
DeployVaultTokenizedBase.sol
Tokenized vault base deployment extension                               

script/base/DeployVaultTokenizedBase.sol

  • Extends base deployment for tokenized vaults
  • Overrides vault version to 2 for tokenized implementation
  • Encodes additional name and symbol parameters
  • Maintains all base functionality with token support
 +43/-0   
Miscellaneous
Logs.sol
Logging utility for deployment scripts                                     

script/base/Logs.sol

  • Utility contract for logging deployment information
  • Writes logs to both console and file system
  • Provides centralized logging functionality
 +16/-0   

@1kresh 1kresh closed this Oct 2, 2025
@1kresh 1kresh reopened this Oct 2, 2025
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Oct 2, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Insecure logging path

Description: The logging function writes to a fixed relative path 'script/logs.txt' by reading and
concatenating existing file content, which can lead to large unbounded files and potential
workspace poisoning if the file is user-controlled; prefer append operations or bounded
logs.
Logs.sol [10-15]

Referred Code 
function log(
    string memory data
) internal {
    console2.log(data);
    vm.writeFile(LOG_FILE, string.concat(vm.readFile(LOG_FILE), data, "\n"));
}
Privilege persistence risk

Description: During deployment with a whitelist, admin and whitelist roles are temporarily assigned to
the deployer and later transferred, but the code relies on vm.readCallers and assertions
rather than enforcing on-chain revocation in all paths, risking lingering privileges if
execution is interrupted before role renouncement.
DeployVaultBase.sol [181-189]

Referred Code 
    IVault.InitParams memory baseParams = params.vaultParams.baseParams;
    baseParams.defaultAdminRoleHolder =
        needWhitelistDepositors ? deployer : params.vaultParams.baseParams.defaultAdminRoleHolder;
    baseParams.depositorWhitelistRoleHolder =
        needWhitelistDepositors ? deployer : params.vaultParams.baseParams.depositorWhitelistRoleHolder;

    return abi.encode(baseParams);
}
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
⚪
No custom compliance provided

Follow the guide to enable custom compliance check.
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Oct 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Consolidate configuration to reduce duplication

To improve maintainability and prevent inconsistencies, consolidate the
duplicated configuration variables from DeployVault.s.sol and
DeployVaultTokenized.s.sol into a shared base contract.

Examples:

script/DeployVault.s.sol [6-54] 
contract DeployVaultScript is DeployVaultBase {
    // Configurations - UPDATE THESE BEFORE DEPLOYMENT

    // Address of the owner of the vault who can migrate the vault to new versions whitelisted by Symbiotic
    address OWNER = 0x0000000000000000000000000000000000000000;
    // Address of the collateral token
    address COLLATERAL = 0x0000000000000000000000000000000000000000;
    // Vault's burner to send slashed funds to (e.g., 0xdEaD or some unwrapper contract; not used in case of no slasher)
    address BURNER = 0x000000000000000000000000000000000000dEaD;
    // Duration of the vault epoch (the withdrawal delay for staker varies from EPOCH_DURATION to 2 * EPOCH_DURATION depending on when the withdrawal is requested)

 ... (clipped 39 lines)
script/DeployVaultTokenized.s.sol [6-58] 
contract DeployVaultTokenizedScript is DeployVaultTokenizedBase {
    // Configurations - UPDATE THESE BEFORE DEPLOYMENT

    // Name of the ERC20 representing shares of the active stake in the vault
    string NAME = "SymVault";
    // Symbol of the ERC20 representing shares of the active stake in the vault
    string SYMBOL = "SV";
    // Address of the owner of the vault who can migrate the vault to new versions whitelisted by Symbiotic
    address OWNER = 0x0000000000000000000000000000000000000000;
    // Address of the collateral token

 ... (clipped 43 lines)

Solution Walkthrough:

Before:

// file: script/DeployVault.s.sol
contract DeployVaultScript is DeployVaultBase {
    // Configurations - UPDATE THESE BEFORE DEPLOYMENT
    address OWNER = 0x...;
    address COLLATERAL = 0x...;
    address BURNER = 0x...;
    uint48 EPOCH_DURATION = 7 days;
    // ... many more duplicated config variables
    
    function run() public { ... }
}

// file: script/DeployVaultTokenized.s.sol
contract DeployVaultTokenizedScript is DeployVaultTokenizedBase {
    // Configurations - UPDATE THESE BEFORE DEPLOYMENT
    string NAME = "SymVault";
    string SYMBOL = "SV";
    address OWNER = 0x...;
    address COLLATERAL = 0x...;
    address BURNER = 0x...;
    uint48 EPOCH_DURATION = 7 days;
    // ... many more duplicated config variables

    function run() public { ... }
}

After:

// file: script/config/DeployVaultConfig.sol (new file)
abstract contract DeployVaultConfig {
    // Configurations - UPDATE THESE BEFORE DEPLOYMENT
    address OWNER = 0x...;
    address COLLATERAL = 0x...;
    address BURNER = 0x...;
    uint48 EPOCH_DURATION = 7 days;
    // ... all common config variables
}

// file: script/DeployVault.s.sol
contract DeployVaultScript is DeployVaultBase, DeployVaultConfig {
    function run() public { ... }
}

// file: script/DeployVaultTokenized.s.sol
contract DeployVaultTokenizedScript is DeployVaultTokenizedBase, DeployVaultConfig {
    string NAME = "SymVault";
    string SYMBOL = "SV";
    function run() public { ... }
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies significant code duplication in configuration across DeployVault.s.sol and DeployVaultTokenized.s.sol, and proposing a base contract is a good architectural improvement for maintainability.

Medium
Possible issue
Add missing role validation checks

Add a missing validation check in _validateOwnershipTransfer to verify that
NETWORK_LIMIT_SET_ROLE has been correctly granted for the
OperatorSpecificDelegator.

script/base/DeployVaultBase.sol [318-329]

 if (params.delegatorParams.baseParams.hookSetRoleHolder != address(0)) {
     assert(
         OperatorSpecificDelegator(delegator).hasRole(
             OperatorSpecificDelegator(delegator).HOOK_SET_ROLE(),
             params.delegatorParams.baseParams.hookSetRoleHolder
         ) == true
     );
 }
+for (uint256 i = 0; i < params.delegatorParams.networkAllocationSettersOrNetwork.length; ++i) {
+    assert(
+        OperatorSpecificDelegator(delegator).hasRole(
+            OperatorSpecificDelegator(delegator).NETWORK_LIMIT_SET_ROLE(),
+            params.delegatorParams.networkAllocationSettersOrNetwork[i]
+        ) == true
+    );
+}
 assert(
     OperatorSpecificDelegator(delegator).operator()
         == params.delegatorParams.operatorAllocationSettersOrOperator[0]
 );
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a missing validation check for NETWORK_LIMIT_SET_ROLE in the deployment script, which improves the script's robustness by ensuring roles are set as expected.

Medium
  • More

@1kresh 1kresh closed this Oct 24, 2025
@1kresh 1kresh deleted the script-updates branch December 8, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Streator @1kresh